cel: complete translator cutover and deadframe integration - #1320
cel: complete translator cutover and deadframe integration#1320youknowone wants to merge 15 commits into
Conversation
|
Warning Review limit reachedNext included review available in 13 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (51)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ed6855157
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| entry[backend] = status | ||
| entry.setdefault("provenance", {})[backend] = dict(cell_axis) | ||
| continue | ||
| if status == shared: |
There was a problem hiding this comment.
Refresh provenance for unchanged baseline cells
When --update-baseline remeasures an existing cell and its status still matches the shared verdict, this branch exits without writing cell_axis. Consequently, almost every pre-existing baseline entry remains permanently provenance-free—or retains an obsolete axis—even after being freshly measured, so axis_drift_report continues reporting it as legacy/drifted. Update the shared cell's provenance here; the analogous unchanged-overlay case below also needs its provenance refreshed.
Useful? React with 👍 / 👎.
81b54fc to
5226c72
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 608c7164fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let arms = assert_portal_installed(interp, expected.is_empty()); | ||
| let degraded = degraded_dispatch_arm_names(interp); | ||
| assert!( | ||
| degraded == expected, |
There was a problem hiding this comment.
Normalize both sides before comparing degraded-arm sets
When a caller pins two or more correct arms in portal/source order—for example, ["ROLL", "ALLOCATE"]—degraded_dispatch_arm_names sorts the actual names alphabetically but this comparison leaves expected untouched, so the documented set assertion panics even though the sets are identical. Normalize the expected names before comparison; assert_degraded_dispatch_arm_causes has the same order-sensitive comparison and should be normalized as well.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit aa0afe9). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
The census test's `JitDriverSpec` literal lists every field explicitly, so it also lists `green_kinds` and `red_kinds`. This driver declares no greens and no reds, so the vectors parallel to them are empty -- which is also the value that leaves the codewriter's kind check disabled. Assisted-by: Claude
The module doc was rewritten and the rewrite dropped the reason the structure has to stay per-op: an earlier version of this machine fused raw_load+add+i++ into one opcode, the clean interpreter became a one-op memory-bandwidth-bound loop with no dispatch left to eliminate, and the JIT read 0.62x. That figure measured the kernel, not the float JIT. `0.62x`, the kernel-versus-JIT attribution, and the `column` probe precedent appear nowhere else in the tree, so nothing carried them after the rewrite. What remained states that the opcodes are separate but not that they must be, which is the half a future reader needs: fusing them back reproduces the 0.62x and invites the same misreading. The rewrite is kept. This adds a paragraph after it rather than reverting to the older wording, and leaves the reworded program list alone -- the op counts there already carry what the dropped `(memory-bound ref)` / `(compute-bound)` annotations said. Doc comments only. Not covered by the rustdoc ratchet, which lists majit-rlib and majit-trace, and carries no rpython/pypy citation for check-citation-drift to validate. Assisted-by: Claude
`majit/examples/dualtape`'s shape-gate doc kept the claim that an op count cannot distinguish body shapes, but not the values it collides on (`1` an empty dispatch, `5` a segmented runaway) nor the record that the doc asserted the count was sufficient until the shape gate refuted it. The `pyre/gate-triage.md` half of this change is gone: it restored detail into the timeout caution in §1b, and `a6684cd6641` retired that section. Assisted-by: Claude
The documented command was `cargo test --all --features dynasm`. With default features on, the workspace's example interpreters default to `cranelift` and supply it to the one shared `majit-metainterp`, while `pyre-jit` compiles in its dynasm registrations — so that command builds a binary that dispatches through one backend and holds the other's hooks. The stated justification, that without the flag `majit-metainterp` emits `compile_error!`, describes a failure that configuration cannot produce: the examples already supply a backend, so the no-backend arm is never reached. Replaced with `--no-default-features --features dynasm` and a reason that matches what the flags do. Assisted-by: Claude
`scripts/check-new-line-citations.py` reads what a change adds and rejects
an `upstream.py:LINE` inside a `//` comment. Of the fourteen it reports for
this branch, eleven are lines already carrying that citation in the base
copy of the same file, which this branch only reindents; three were written
here.
Each of the three already named, or sits next to, a symbol the number was
redundant with: `rclass.py:162-165` quotes `OBJECT = GcStruct('object', ...`,
which occurs once in that file; `descroperation.py:706-712` is `binop_impl`,
now named because the quote beside it occurs twice; `policy.py:48-84` already
spelled `look_inside_graph`.
Assisted-by: Claude
…mbedders Two things every interpreter built on this framework writes for itself. `intrinsics` holds the untraced bodies of the raw-memory, bitcast and unsigned-arithmetic intrinsics the `#[jit_interp]` lowerer rewrites while tracing. The lowerer matches the LAST segment of the call expression's path and never looks at where the function is defined, so an imported name is recognized exactly as a local `fn` of the same name is. Two tests in `lower_value.rs` say so: `an_imported_intrinsic_lowers_like_a_local_one` lowers the bare and the fully-qualified spelling of a load, a store and an unsigned compare and asserts the two are equal, and `only_the_last_path_segment_selects_an_intrinsic` is its control -- without it, "the qualified spelling lowered the same" would also hold of a lowerer that ignored the path and matched on argument shape alone. A spelling that failed to be recognized would not error. It would lower as a residual call, which is a worse graph and not a broken one, so the property needs a test rather than a reading. `embed::Census` holds the counters an embedder tallies off the four driver callbacks, installs itself onto a driver, and reads the abort-reason counters as a DELTA. The delta is the reason the type exists: the counters are process-global, so "since process start" answers a different question from "during this run", and two overlapping consumers of a resetting read each see a fraction of the truth. Its serializing lock is a process-global `Mutex` and not a `thread_local!`, which would serialize nothing. `examples/tinyframe` is ported onto both as the proof, losing 93 lines of its own copies and gaining 43. Neither module has a same-named upstream file, so both are declared in the local-boundary list in `lib.rs` with the reason: RPython needs no intrinsics module because it lowers `rawstorage.py`, `longlong2float.py` and `rarithmetic.py` at rtype time from the one definition each already has, and the census's upstream equivalent is split between `warmspot.py`'s test harness and `jitprof.py`, neither of which is a surface an embedder calls. Assisted-by: Claude
`assert_no_degraded_dispatch_arms` asserts the degraded set is empty. A machine with a known lowering gap has a non-empty set, so the assertion fails on it from the first call and the twelve examples that filter `degraded_dispatch_arms()` by hand all wrote their own check instead. `embed::assert_degraded_dispatch_arms` takes the expected set, so a known gap is expressible: a new name means an arm stopped lowering, a missing name means the pinned gap closed. `assert_no_degraded_dispatch_arms` is now its `&[]` case. `assert_degraded_dispatch_arm_causes` pins each arm's refusal family and `assert_degraded_dispatch_arm_reason_contains` pins the source it refuses on; `degraded_dispatch_arm_names`, `degraded_dispatch_arm_causes` and `degraded_dispatch_arm_reason` read the registry without asserting. Both assertions look up the arm census first, so an uninstalled portal reports that rather than reading as a closed gap. spcount, tla and tlr drop their hand-rolled compile counters, shape flags and probe mutex for `embed::Census` and the new assertion. Assisted-by: Claude
…xes back to it `seed_bridge_virtualizable_boxes` rebuilds the virtualizable shadow out of a guard's fail args and stopped there, on the stated premise that every `[.. ; virt]` state field is `RustVec`-stored and so carved out of `synchronize_virtualizable`'s write-back. A state field holding a pointer to a `[length][payload…]` block is not carved out, and for one of those the live state struct kept the values the compiled run was entered with while the shadow held the values at the guard. Port `rebuild_state_after_failure`'s trailing `synchronize_virtualizable()` for a vinfo with no `vable_token`. Also correct the freeze premise in the `writeback_virt_array_state_fields` comment, which named the same carve-out. Assisted-by: Claude
…a terminal return A guard-resume bridge whose walk reaches the interpreted function's return sets `single_pass_finish` and reports `usize::MAX` as its position. The `jit_merge_point!` expansion reads that flag and `break`s; the `can_enter_jit!` expansion did not, and assigned the position to the loop's `pc`, which indexes out of bounds in a dispatch loop that is not bottom-tested on its program length. Emit the same `break` at the back edge, and leave the flag standing in `bridge_from_guard_resume_position` for the caller to consume. Assisted-by: Claude
`seed_jitframe_pool_arm` now answers pooled when `MAJIT_JITFRAME_POOL` is unset, and reads `0` as a request for the owned arm, so the variable selects either arm rather than only switching the pool on. `set_jitframe_pool` is unchanged and still overrides it in both directions. Doc comments updated to name the shipped arm: `FrameHeapOwner`'s summary, the `pooled` field, `FrameHeapOwner::new`, `set_jitframe_pool`, the allocation site in `run_compiled_code_inner`, and `count_owned_frame_buf`, whose thread-local access and `RefCell` borrow per allocation are now charged to the probe arm. Adds the `MAJIT_JITFRAME_POOL` entry to `majit/gate-triage.md`. Assisted-by: Claude
…rrect what it is for `do_compile` gated `ctx.set_disasm(true)` and a `[jit][disasm-body]` print on `MAJIT_DUMP_CLIF` for the trace body only. Adds the symmetric pair for the host-callable `trace_N_entry` wrapper: `set_disasm` on its context, a `[jit][code-size] wrapper_bytes=` line from the `wrapper_code_bytes` already computed there, and `[jit][disasm-entry]`. Corrects the comment above `body_call_conv`. It stated that Tail "clobbers AppleAarch64 callee-saves x19-x28/x29" and gave that as the reason the wrapper exists. `get_regs_clobbered_by_call` answers `ALL_CLOBBERS` for `(Tail, true)`, the exception path, which emits no exception tables here; an ordinary Tail call takes `DEFAULT_AAPCS_CLOBBERS` and preserves x19-x28. The wrapper's reason is the pinned register, which `enable_pinned_reg` makes non-callee-saved in Cranelift while AAPCS treats it as callee-saved. Records what the emitted wrapper measures on aarch64: 13 instructions, 52 bytes, 4 memory ops over x19 and the fp/lr frame record, no callee-save block and no fp saves, noted as an upper bound rather than a measured cost. Updates the `MAJIT_DUMP_CLIF` read-site count in `majit/gate-triage.md` from two to four. Assisted-by: Claude
…stages and a residual Adds `back-edge-stage-probe`, off by default, under which `JitDriver::back_edge_internal` carries run-time selectable amplification loops for three stages of a warm entry: the gate (`entry_procedure_token` on the uncarried shape, `get_compiled_meta().cloned()`, `driver_descriptor_for`, `is_compatible`), marshal-in (`sync_before`, `extract_live_values_into`, `live_values_match_descriptor`, `extend_compiled_live_values_into` and the buffer clears), and marshal-out (`restore_values`, `sync_after`). `execute_assembler_at_dispatch_key` is not amplified and is documented as the residual: it runs the trace, so a second pass would execute it again from state the first advanced. `get_latest_descr_arc` and `decode_exit_slots` are inside it, so the deadframe decode is in that residual too. The one-shot moves left out of the three stages — `resolve_cell_key`, the single-pass dispatch key take, `take_entry_scratch`'s `mem::take`, `entry_scratch_out`, and `drop(result)` — are named on the fields that exclude them. `BackEdgeStageRepeats` packs four u16 counts into one `AtomicU64`, so the entry path takes one relaxed load that every arm pays. `set_back_edge_stage_repeats` is the only selector; there is no environment gate, since all-zero is the shipping shape. `back_edge_stage_passes` reports amplified passes per stage. The marshal-in loop runs ahead of the shipping refill and ends each pass with the clears `take_entry_scratch` performs, so the buffers the real extraction fills are in the state it would have found them in. Marshal-out runs before the arm split and skips `restore_values` for a FINISH, as that arm does. Assisted-by: Claude
Adds a `execute-stage-probe` cargo feature to majit-metainterp (forwarded to majit-backend-cranelift) that subdivides the stage `back_edge_internal` spends inside `execute_assembler_at_dispatch_key`. Four sub-stages, measured two different ways: - prologue (`compiled_loops` probe, meta clone, `prepare_compiled_run_io`) and deadframe decode (`get_latest_descr_arc`, the fail-descr reads, `decode_exit_slots`) are repeatable, so they are AMPLIFIED: each is re-run `repeats` extra times and the difference over `repeats` is the stage. An empty barrier loop carrying the same `black_box` barrier subtracts the amplification's own cost. - `execute_token_with_dispatch_key` runs the trace and consumes its arguments, so it is not repeatable. It is SINGLE-SHOT: clocked once per entry into `CALL_SHOT_NS`/`CALL_SHOT_COUNT`, with `execute_stage_clock_floor_ns` measuring this box's `Instant` floor for the caller to subtract. - The jitframe build and input writes happen inside `run_compiled_code_inner`, i.e. INSIDE the single-shot call, not beside it. `set_frame_build_repeats` amplifies them against a scratch `FrameHeapOwner` on the non-`use_gc_alloc` arm. It is an "of which" figure and is not a summand of the stage total. `ExecuteStageRepeats` packs the four counts into one process-global `AtomicU64`; every arm pays the same relaxed load, so it cancels out of their difference. All-zero is the shipping shape and no env var reads it, so no gate-triage row is minted. With the feature off the crates build byte-identical. Assisted-by: Claude
…the flags The sibling-door yield scan an embedder runs ahead of a warm entry asks `has_runnable_compiled_loop`, which is a conjunction over three state sources. Two of them are fused in the public API: `has_compiled_loop` is `entry_procedure_token(..).is_some()`, and `JitCell::get_procedure_token` performs the `Weak::upgrade` AND reads `invalidated` inside one function. So no arm built from the public API can separate what the refcount pair costs from what the flag reads cost. `WarmState::probe_cell_token_upgrades` is that separation: the cell lookup, the upgrade, and the drop of the `Arc` it produced, with no flag read after it. Differenced against `has_compiled_loop`, it leaves the flags. Its answer is deliberately WEAKER than the shipping predicate's -- an invalidated token still upgrades, so it returns true where the door decides false. It is a cost probe and never a decision, and the doc says so at all three levels. Behind `yield-stage-probe`, off by default, no production caller. Wrappers on `MetaInterp` and `JitDriver` carry the same gate, so a default build has neither the reader nor its symbol. Assisted-by: Claude
…ion diamond
`core::slice::<Impl>::get` is a foreign leaf whose body is Opaque in the
LLBC, so it residualizes as an unregistered callee and the rtyper Skips
every calling graph. `front::slice_get` rewrites the residual call into
`if i < len(slice) { Some(slice[i]) } else { None }`, the general case of
the `front::slice_first` diamond.
`recognize_slice_get_site` pins the scalar `SliceIndex` instantiation by
the index operand's declared `usize`: `get(0..2)` returns `Option<&[T]>`,
and an element read at the range's start would hand the consumer a `T`
where a `[T]` is expected. Every range form falls through and keeps its
residual call. The rewrite is fail-safe throughout — a structural
mismatch returns `Err` and leaves the call untouched.
The `front/mir.rs` plumbing (site vector, capture site,
`recognize_slice_get_site`, post-pass driver, reachability gate) is not
included; that file carries another change in progress. Until it lands
the pass is compiled and never invoked, which is what the three
dead-code warnings report.
Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa0afe90a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub fn majit_raw_load_i8(base: i64, ea: i64) -> i64 { | ||
| unsafe { core::ptr::read_unaligned(raw_addr(base, ea) as *const i8) as i64 } |
There was a problem hiding this comment.
Mark raw-pointer intrinsics unsafe
When any safe Rust caller supplies a null, dangling, or otherwise invalid address, this public function executes read_unaligned and causes undefined behavior; documentation cannot transfer that safety obligation from a safe API to its callers. The same issue affects every public raw-load and raw-store function in this module, so they should be unsafe and the macro lowerer should preserve calls through unsafe blocks rather than weakening the interpreter API to accommodate the tracer.
AGENTS.md reference: AGENTS.md:L12-L15
Useful? React with 👍 / 👎.
| pub(crate) fn rewire_slice_get_call_sites( | ||
| graph: &mut FunctionGraph, | ||
| sites: &[SliceGetSite], | ||
| ) -> usize { |
There was a problem hiding this comment.
Connect the slice-get pass to production lowering
For every translated body that calls <[T]>::get, this rewrite is currently unreachable: a repository-wide search at this commit finds SliceGetSite, rewire_slice_get_call_sites, and the claimed recognize_slice_get_site only in this new file, with all invocations confined to its unit tests. Consequently production MIR lowering never records or rewrites a site, so the residual core::slice::get call remains and its caller continues to be skipped by the rtyper rather than receiving the advertised bounds-checked diamond.
AGENTS.md reference: AGENTS.md:L206-L209
Useful? React with 👍 / 👎.
What
Why
This is the CEL-specific remainder after extracting the reusable majit work into the base branch. Keeping it as one commit directly above
majit-runtime-followupmakes the merge order explicit and allows a linear merge after the base PR lands.Stack
majit-runtime-followup(must land first)Checks
majit-rlib,pyre-object,pyre-interpreter, andpyre-jitcargo check --features dynasmcargo test --features dynasmcargo test --all --no-default-features --features dynasmcargo test --release -p cel --no-default-features --features dynasm(5/5)python3 scripts/check-citation-drift.py --self-testRUSTDOCFLAGS='-D warnings' cargo doc --no-deps --locked -p majit-rlib -p majit-tracepython3 pyre/check.py --backend dynasm --no-synthetic(dynasm 10/10;fib_recursivecounters were unstable and non-gated on this run)